Fix race conditions in Regex.cc. #11399
Conversation
ywkaras
force-pushed
the
regex_crash
branch
4 times, most recently
from
c6713d5 to
976c730
Compare
These changes should prevent (low-frequency) crashes that are being seen in Yahoo Prod.
|
Should this be added to 10.0.x project as well for cherry-pick? |
Debatable. Any crashes would happen at shutdown, and seem infrequent (with limited testing in Yahoo Prod). |
|
This is where the segment violation is occurring in Yahoo Prod: trafficserver/src/tsutil/Regex.cc Line 114 in 5f52c37 |
There was a problem hiding this comment.
Could RegexContext be automatic within the thread? i.e.
RegexContext* get_instance() {
thread_local RegexContext ctx;
return &ctx;
}
Why is the global vector of contexts necessary at all?
IIRC this was originally done because some Debug statements in destructors could come after the shutdown of the thread's regex context. But if we use Dbg in those locations, then this isn't a problem anymore?
|
Could also look at how Metrics storage lifetime outlasts the threads and see if this would work for _Data lifetime: https://github.com/apache/trafficserver/blob/master/src/tsutil/Metrics.cc#L34-L42 |
|
Yes, it would not be a logic error in creating automatic, function scope instances of RegexContext when one is needed. The issue is whether the performance hit is tolerable. On the other hand, this change involves multiple threads/cores hardware locking a variable in cache, which is also a potentially significant performance hit. |
Because of trafficserver/src/tsutil/DbgCtl.cc Line 161 in 0a53513 I guess I should look into changing DbgCtl:::_new_reference() so it doesn't call tag_activated() with its mutex locked. |
|
Closing this because I think #11416 is better. |
3 participants
These changes should prevent (low-frequency) crashes that are being seen in Yahoo Prod.